-
Notifications
You must be signed in to change notification settings - Fork 13.8k
[WIP] mgca: Add ConstArg representation for const items #139558
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
This comment has been minimized.
This comment has been minimized.
fa42f86
to
6054bd5
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
ac73a4a
to
4f6c9ab
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
☔ The latest upstream changes (presumably #141343) made this pull request unmergeable. Please resolve the merge conflicts. |
@oli-obk I've assigned you to this PR alongside me because I'd definitely want you to review it before it lands since its so CTFE involved 🤔 I don't think it needs reviewing rn though, things are so up in the air and we're not bootstrapping yet :3 |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
b567bdb
to
e1a0a9d
Compare
This comment has been minimized.
This comment has been minimized.
e1a0a9d
to
40e5b35
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
)? { | ||
TypeRelativePath::AssocItem(def_id, args) => { | ||
if !tcx.associated_item(def_id).is_type_const_capable(tcx) { | ||
if !find_attr!(self.tcx().get_all_attrs(def_id), AttributeKind::TypeConst(_)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know why find_attr!
works here but tcx.has_attr(sym::type_const)
doesnt. cc @jdonszelmann does has_attr
work with new style attributes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://doc.rust-lang.org/stable/nightly-rustc/src/rustc_hir/hir.rs.html#1283 seems like no new style attributes don't work with has_attr
if it's been parsed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it works sometimes though because we added has_attr
calls in some places to fix ICEs which worked.
edit: ah no it probably just made that code unreachable lmao
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
pub generics: Generics, | ||
pub ty: Box<Ty>, | ||
pub expr: Option<Box<Expr>>, | ||
pub body: Option<ConstItemRhs>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pub body: Option<ConstItemRhs>, | |
pub rhs: Option<ConstItemRhs>, |
body terminology doesn't make too much sense since it's only sometimes a body
compiler/rustc_hir/src/hir.rs
Outdated
Static(Mutability, Ident, &'hir Ty<'hir>, BodyId), | ||
/// A `const` item. | ||
Const(Ident, &'hir Generics<'hir>, &'hir Ty<'hir>, BodyId), | ||
// TODO: make sure we only allow usage of path RHS in generic contexts under mgca, not stable! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is now done, uncertain whether we may want some kind of ast validator or hir visitor to assert this :3
200e7a1
to
43a38ab
Compare
This comment has been minimized.
This comment has been minimized.
This was a pre-existing bug that was not exposed by the test suite. We need a layer of branches for each level of the data, and since CStrs are a wrapper around a slice, we were missing a branch here.
43a38ab
to
f21c74e
Compare
This comment has been minimized.
This comment has been minimized.
f21c74e
to
277c69a
Compare
The job Click to see the possible cause of the failure (guessed by this bot)
|
r? @BoxyUwU